fix(facets): export label and count columns in facet CSV download#864
Open
thostetler wants to merge 3 commits intoadsabs:masterfrom
Open
fix(facets): export label and count columns in facet CSV download#864thostetler wants to merge 3 commits intoadsabs:masterfrom
thostetler wants to merge 3 commits intoadsabs:masterfrom
Conversation
Member
Author
|
@shinyichen should this one be CSV? Let me know if these changes makes sense. Thanks! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #864 +/- ##
========================================
+ Coverage 62.7% 62.7% +0.1%
========================================
Files 323 323
Lines 38011 38029 +18
Branches 1723 1724 +1
========================================
+ Hits 23802 23821 +19
+ Misses 14169 14168 -1
Partials 40 40
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Risk summary: Medium risk — behavior is straightforward (CSV formatting + download), but the intended CSV MIME type is not actually applied due to how useDownloadFile constructs the Blob.
Changes:
- Added a pure
formatFacetCSVhelper to export facet data asLabel,CountCSV with basic RFC-style escaping. - Updated facet modal download to use CSV output and a
.csvfilename. - Added unit tests covering CSV formatting behavior and escaping.
Findings (priority order)
- medium — Incorrect MIME type for CSV download (bug)
- Impact: Downloaded file may not be recognized/handled as CSV by browsers/OS tooling (despite
.csvextension), contradicting the PR’s stated goal of settingtext/csv. - Location:
src/components/SearchFacet/SearchFacetModal/SearchFacetModal.tsx:202-203 - Minimal fix: Update
useDownloadFileto use the MIME returned bygetMetaData(type)when creating the Blob (instead of passing theFileTypestring as the Blobtype), or otherwise allow passing an explicit MIME type. - Confidence: high
- Impact: Downloaded file may not be recognized/handled as CSV by browsers/OS tooling (despite
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/SearchFacet/SearchFacetModal/SearchFacetModal.tsx | Switches facet full-list download to CSV output via shared helper and .csv filename. |
| src/components/SearchFacet/helpers.ts | Adds formatFacetCSV(items) helper to generate Label,Count CSV with escaping. |
| src/components/SearchFacet/tests/helpers.test.ts | Adds unit tests for formatFacetCSV (header, hierarchy stripping, counts, escaping). |
Comment on lines
+202
to
+203
| const formatData = useCallback(() => formatFacetCSV(treeData), [treeData]); | ||
| const { onDownload } = useDownloadFile(formatData, { filename: 'fulllist.csv', type: 'CSV' }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Facet modal download only exported the label column — the count was never included it seems?
formatFacetCSVas a pure helper that emits a proper two-column CSV (Label,Count)FacetDownloadButtonto use the helper; changes filename tofulllist.csvand MIME type totext/csvCloses SCIX-872